[core] Add DDS support for {text,icon}-size #8593

anandthakker · 2017-03-31T04:11:43Z

Work through render test errors and segfaults ( 😱 )
Review SymbolSizeData / SymbolSizeAttributes design ( @jfirebaugh could I get your take on this? Main place to look/start is probably top of symbol_program.cpp, SymbolLayout::addSymbol, and maybe makeValues())
Refactor SymbolProgram, currently just copy-pasted from Program to make for easier initial hacking.
Correct naive linear interpolation to make use of exponential base.
Smaller TODOs
Request full review

jfirebaugh · 2017-03-31T21:50:02Z

src/mbgl/programs/symbol_program.hpp

+        : sizePropertyValue(size),
+          layoutZoom(tileZoom + 1),
+          defaultSize(defaultSize_) {
+        size.match(


Some pathways don't initialize layoutSize. Should it have a default value or be typed as optional<float>?

jfirebaugh · 2017-03-31T21:58:32Z

src/mbgl/programs/symbol_program.hpp

+struct SymbolSizeAttributes : gl::Attributes<attributes::a_size> {
+    using SourceFunctionVertex = gl::detail::Vertex<gl::Attribute<uint16_t, 1>>;
+    struct monostate {};
+    using VertexVector = variant<


Instead of having parallel VertexVector and VertexBuffer variants, and three different match statements (in attributeBindings, SymbolSizeData, and upload), what about following a model like PaintPropertyBinder, where an initial match statement then transfers control to a polymorphic object, whose derived types each hold both VertexVector and VertexBuffer of the appropriate matching types?

Ah, yeah! I started out thinking that duplicating a PaintPropertyBinder hierarchy like that would be overkill, but actually it could be pretty minimal/scoped and, like you say, cleaner than having all these different matches. 🙇

This fixes a bug where, for a zoom value greater than that of the highest zoom stop, composite function interpolation would return nan. (Blocking a render test over in #8593) * Add failing tests for composite function edge case The failing cases here are: - Should interpolate before the first stop - Should interpolate past the last stop * Fix edge case in composite function interpolation * Hold functions constant outside stop-defined domain

anandthakker · 2017-04-04T17:41:53Z

@jfirebaugh refactored to a paintpropertybinders-like polymorphic approach f219d8c, as you suggested. It's a few more LOC, but it does feel like a cleaner design. The repetition of the PaintPropertyBinders hierarchy makes me itch to refactor the redundancy, but it would probably be premature abstraction to do it just yet; I think we should wait till we need a third copy.

anandthakker · 2017-04-04T17:48:23Z

Refactor SymbolProgram, currently just copy-pasted from Program to make for easier initial hacking

I think the main issue here is to dedupe the identical constructors of SymbolProgram and Program (e.g.: https://github.com/mapbox/mapbox-gl-native/pull/8593/files#diff-5907166344b0d46b118326c98eb9915cR312). They're really just initializing a gl::Program, so I think that logic can be extracted into a static factor function or maybe even just a constructor for gl::Program.

jfirebaugh · 2017-04-05T18:53:38Z

src/mbgl/gl/program.hpp

@@ -37,6 +41,57 @@ class Program {
          attributeLocations(Attributes::loadNamedLocations(binaryProgram)),
          uniformsState(Uniforms::loadNamedLocations(binaryProgram)) {
    }
+
+    template <class Shaders>


To reduce binary size, pass vertexSource, fragmentSource, and name as parameters rather than templating on Shaders.

jfirebaugh · 2017-04-05T18:55:15Z

src/mbgl/programs/attributes.hpp

 // Layout attributes

 MBGL_DEFINE_ATTRIBUTE(int16_t, 2, a_pos);
 MBGL_DEFINE_ATTRIBUTE(int16_t, 2, a_extrude);
 MBGL_DEFINE_ATTRIBUTE(int16_t, 4, a_pos_offset);
 MBGL_DEFINE_ATTRIBUTE(uint16_t, 2, a_texture_pos);

-template <std::size_t N>
+template <std::size_t N, typename T>


Flip the order of T and N, so that it matches gl::Attribute.

anandthakker · 2017-04-05T21:44:36Z

@jfirebaugh updated to address your comments and also rebased + cleaned up the history a bit. Should be ready for a full review now.

Ports mapbox/mapbox-gl-js#4455

anandthakker added Core The cross-platform C++ core, aka mbgl ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Mar 31, 2017

anandthakker mentioned this pull request Mar 31, 2017

Enable property functions for {text,icon}-size mapbox/mapbox-gl-js#4455

Merged

12 tasks

anandthakker force-pushed the dds-text-size branch 2 times, most recently from a85307b to 545ca67 Compare March 31, 2017 15:03

jfirebaugh self-requested a review March 31, 2017 20:16

jfirebaugh reviewed Mar 31, 2017

View reviewed changes

anandthakker mentioned this pull request Apr 1, 2017

Fix edge case in composite function interpolation #8613

Merged

anandthakker force-pushed the dds-text-size branch from 545ca67 to c85262f Compare April 1, 2017 19:32

anandthakker force-pushed the dds-text-size branch from f89b3e8 to 7cfa1f2 Compare April 3, 2017 19:30

jfirebaugh reviewed Apr 5, 2017

View reviewed changes

boundsj added this to the ios-v3.6.0 milestone Apr 5, 2017

anandthakker force-pushed the dds-text-size branch 2 times, most recently from 66b27fb to 8af6500 Compare April 5, 2017 21:11

anandthakker force-pushed the dds-text-size branch 2 times, most recently from bb0463d to 24f10e8 Compare April 6, 2017 11:40

jfirebaugh approved these changes Apr 6, 2017

View reviewed changes

Anand Thakker added 8 commits April 6, 2017 14:59

Update gl-js and generate style code

264109b

Factor out packUint8Pair() helper function

2dd46a8

Draft implementation of DDS for {text,icon}-size

7d8d2d8

Ports mapbox/mapbox-gl-js#4455

Fix text-size/composite-function-line-placement test

c803b0a

Refactor to PaintPropertyBinders-like strategy

e59e1be

Dedupe gl::Program construction

db1cc28

Use exponential function base for interpolation

4b884e2

Dedupe coveringZoomStops method

583116e

Anand Thakker added 2 commits April 6, 2017 14:59

Fixup tests

926a9da

Fix CI errors (hidden within #if block)

10e1fc1

anandthakker force-pushed the dds-text-size branch from 24f10e8 to 10e1fc1 Compare April 6, 2017 18:59

anandthakker removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Apr 6, 2017

anandthakker merged commit 693c9f3 into master Apr 6, 2017

anandthakker deleted the dds-text-size branch April 6, 2017 19:30

jfirebaugh mentioned this pull request Apr 6, 2017

Master ticket for remaining data-driven style properties mapbox/mapbox-gl-js#4434

Open

1ec5 mentioned this pull request Apr 12, 2017

Update and reorganize changelogs #8723

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Add DDS support for {text,icon}-size #8593

[core] Add DDS support for {text,icon}-size #8593

anandthakker commented Mar 31, 2017 •

edited

Loading

jfirebaugh Mar 31, 2017

jfirebaugh Mar 31, 2017

anandthakker Apr 1, 2017

anandthakker commented Apr 4, 2017

anandthakker commented Apr 4, 2017

jfirebaugh Apr 5, 2017

jfirebaugh Apr 5, 2017

anandthakker commented Apr 5, 2017

[core] Add DDS support for {text,icon}-size #8593

[core] Add DDS support for {text,icon}-size #8593

Conversation

anandthakker commented Mar 31, 2017 • edited Loading

jfirebaugh Mar 31, 2017

Choose a reason for hiding this comment

jfirebaugh Mar 31, 2017

Choose a reason for hiding this comment

anandthakker Apr 1, 2017

Choose a reason for hiding this comment

anandthakker commented Apr 4, 2017

anandthakker commented Apr 4, 2017

jfirebaugh Apr 5, 2017

Choose a reason for hiding this comment

jfirebaugh Apr 5, 2017

Choose a reason for hiding this comment

anandthakker commented Apr 5, 2017

anandthakker commented Mar 31, 2017 •

edited

Loading